Fix/252 polygon holes subgroup by Nishita-shah1 · Pull Request #326 · animint/animint2

Nishita-shah1 · 2026-05-17T21:08:06Z

Closes #252

creating pr from branch instead of from fork

Refactor polygon rendering to use d3.nest for subgroup aesthetics and GeoJSON with evenodd fill rule.

codecov · 2026-05-17T21:56:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.13%. Comparing base (00f0d7b) to head (d843774).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
- Coverage   73.01%   69.13%   -3.89%     
==========================================
  Files         164      163       -1     
  Lines        8835     6000    -2835     
==========================================
- Hits         6451     4148    -2303     
+ Misses       2384     1852     -532

Flag	Coverage Δ
javascript	`?`
r	`69.13% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tdhock

can you also please post screenshots of the test cases?

tdhock · 2026-05-20T17:56:02Z

+  path.list <- getNodeSet(html, '//g[contains(@class,"geom")]//path')
+  expect_true(length(path.list) >= 1)
+  d.vals <- sapply(path.list, function(node) xmlGetAttr(node, "d"))
+  expect_true(


can you please avoid expect_true, and use expect_ something more specific like equal?

Sure, I'll update it.

Nishita-shah1 · 2026-05-20T18:35:25Z

can you also please post screenshots of the test cases?
Hi @tdhock ,
these are the screenshots of the test-renderer-polygon-holes.R file test cases.

tdhock · 2026-05-22T14:51:22Z

I tried this small-scale real data application, https://github.com/tdhock/hicream-viz/blob/main/data-2025-10-09/figure-pixels-chr1-Cluster73-fix.R and this PR works!

top: this PR.
bottom: master branch.

tdhock · 2026-05-22T16:06:57Z

here is a rendered animint using this PR https://tdhock.github.io/2026-05-22-HiC-pixels-chr1-Cluster73-subgroup/

and the same animint which does not render correctly using current master https://tdhock.github.io/2026-05-22-HiC-pixels-chr1-Cluster73-bad/

this is evidence of a successful implementation, great work, thanks very much Nishita!

tdhock · 2026-05-22T16:30:48Z

currently there is only one clickID in tests. are these tests strong enough to fail on current master?

the issue #252 had a screenshot of master drawing the test case like this:

we should add a clickID or mouseMove to the test so that it fails when drawn with current master.
for example, if we put clickSelects and showSelected on the polygon, and draw a red dot behind the polygon in the area that it is incorrectly drawn (hold_and_mid_mid_bottom), then clickID on the red dot, it will actually click on the polygon, which will hide the polygon, and so we should add an expectation that the polygon is still visible even after that click. Does that make sense?

tdhock · 2026-05-22T19:06:42Z

+library(testthat)
+library(animint2)
+library(XML)
+context("Polygon holes via subgroup aesthetic")
+tests_init()


please delete all this from test files

tdhock · 2026-05-22T19:07:19Z

+## hole_and_mid: outer ring + hole + island (3 subgroups)
+## only_hole:    outer ring + hole (2 subgroups)
+## no_hole:      outer ring only (1 subgroup)
+make.full.data <- function(){


it is not necessary (and potentially confusing) to create a function if you only call it once.

tdhock · 2026-05-22T19:22:28Z

+    0,1,0,0,1,0,
+    0,1,1,1,1,0,
+    0,0,0,0,0,0), 6, 6, byrow=TRUE)
+  res <- isoband::isobands(


we should use contourLines() instead of isobands() so we can avoid adding a dependency.

Nishita-shah1 · 2026-05-23T12:32:45Z

currently there is only one clickID in tests. are these tests strong enough to fail on current master?

the issue #252 had a screenshot of master drawing the test case like this: we should add a clickID or mouseMove to the test so that it fails when drawn with current master. for example, if we put clickSelects and showSelected on the polygon, and draw a red dot behind the polygon in the area that it is incorrectly drawn (hold_and_mid_mid_bottom), then clickID on the red dot, it will actually click on the polygon, which will hide the polygon, and so we should add an expectation that the polygon is still visible even after that click. Does that make sense?

yes, we can do that. I'll add some more tests.

Nishita-shah1 · 2026-05-23T12:39:58Z

here is a rendered animint using this PR https://tdhock.github.io/2026-05-22-HiC-pixels-chr1-Cluster73-subgroup/

and the same animint which does not render correctly using current master https://tdhock.github.io/2026-05-22-HiC-pixels-chr1-Cluster73-bad/

this is evidence of a successful implementation, great work, thanks very much Nishita!

Thank you @tdhock , I surely update the tests based on the feedbacks and correct the naming of the tests.

tdhock · 2026-05-23T12:40:38Z

actually can you please just close this PR and port your new JS code to #328 ?

tdhock · 2026-05-23T12:41:11Z

please do keep those review comments in mind for future PRs though.

Nishita-shah1 · 2026-05-23T12:50:59Z

Sure @tdhock , I'll close this PR and port to #328 .
Yes, I'll keep in mind the review comments for future PRs.

nishita-shah1 and others added 4 commits April 4, 2026 19:57

fix issue #252, support subgroup aesthetic in geom_polygon for holes

3d5740b

Merge branch 'master' into fix/252-polygon-holes-subgroup

7c84313

Refactor polygon rendering for subgroup aesthetics

664157a

Refactor polygon rendering to use d3.nest for subgroup aesthetics and GeoJSON with evenodd fill rule.

Merge branch 'master' into fix/252-polygon-holes-subgroup

d843774

This was referenced May 19, 2026

fix issue #252, support subgroup aesthetic in geom_polygon for holes #320

Closed

Nishita's GSoC 2026 updates #322

Open

tdhock requested changes May 20, 2026

View reviewed changes

tdhock mentioned this pull request May 22, 2026

tooltipID() helper for testing polygon hole #328

Draft

tdhock reviewed May 22, 2026

View reviewed changes

Conversation

Nishita-shah1 commented May 17, 2026 • edited by tdhock Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tdhock left a comment

Choose a reason for hiding this comment

Uh oh!

tdhock May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Nishita-shah1 May 23, 2026

Choose a reason for hiding this comment

Uh oh!

Nishita-shah1 commented May 20, 2026

Uh oh!

tdhock commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdhock commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdhock commented May 22, 2026

Uh oh!

tdhock May 22, 2026

Choose a reason for hiding this comment

Uh oh!

tdhock May 22, 2026

Choose a reason for hiding this comment

Uh oh!

tdhock May 22, 2026

Choose a reason for hiding this comment

Uh oh!

Nishita-shah1 commented May 23, 2026

Uh oh!

Nishita-shah1 commented May 23, 2026

Uh oh!

tdhock commented May 23, 2026

Uh oh!

tdhock commented May 23, 2026

Uh oh!

Nishita-shah1 commented May 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Nishita-shah1 commented May 17, 2026 •

edited by tdhock

Loading

codecov Bot commented May 17, 2026 •

edited

Loading

tdhock commented May 22, 2026 •

edited

Loading

tdhock commented May 22, 2026 •

edited

Loading